fix: polish github_actions_organization_workflow_permissions for merge readiness#3281
fix: polish github_actions_organization_workflow_permissions for merge readiness#3281austenstone wants to merge 2 commits intointegrations:mainfrom
Conversation
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
@austenstone I'm not sure why you created this instead of commenting on #3222? Could you please elaborate on the following?
restores the conflict-specific enterprise restriction error handling
Why? This is an undocumented error path that introduces additional complexity for no real value (working from memory).
uses id as the canonical lookup key in read/update/delete
This is an anti-pattern that we've been working hard to remove, in plugin v2 the ID is special string field that we populate, occasionally update, but don't use, especially when the value is already in an actual schema field.
switches import back to passthrough
Why, passthrough writes leave the resource in an inconsistent state.
does read-after-write on create/update so state is authoritative
This is very much an anti-pattern, especially in this context where the GitHub API rate limit is positively anemic.
|
Fair call — that’s on me. I was batch-reviewing and didn’t dig into this one enough before opening the follow-up. I’m closing this out and will keep any input on #3222 scoped to the bool fix. |
2 participants
Summary
This PR carries forward the
github_actions_organization_workflow_permissionsfix from #3222 and applies the additional cleanup needed to make it fit the existing provider patterns more cleanly.What changed
can_approve_pull_request_reviews = falseidas the canonical lookup key in read/update/deleteWhy
PR #3222 fixes the core bug, but it also introduced a few shape changes that made the resource less consistent with adjacent resources in this provider and dropped the clearer enterprise-conflict error path.
This branch keeps the important behavior fix while tightening the implementation so it is easier to merge as-is.
Validation
go test ./github -run TestAccGithubActionsOrganizationWorkflowPermissions -count=1Related